Issues/8607/review missed #9259
Conversation
WalkthroughThe changes in this pull request update the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 eslintsrc/components/ui/badge.tsx[error] 15-16: Delete (prettier/prettier) 🔇 Additional comments (4)src/components/ui/badge.tsx (4)
The documentation clearly explains the component's purpose and includes practical examples specifically demonstrating the review status use cases, which aligns well with the PR objectives.
The variant definitions are well-thought-out:
The component implementation successfully incorporates the previously suggested accessibility improvements, including proper ARIA attributes and TypeScript types.
The Badge component looks well-implemented for the review status feature. Let's verify its integration: ✅ Verification successfulBadge component is correctly integrated with ManagePatients.tsx The Badge component is properly integrated and used as intended:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check Badge component usage in ManagePatients.tsx
# Expect to find usage with 'purple' variant for missed reviews and 'success' for upcoming reviews
# Search for Badge usage in ManagePatients.tsx
rg -A 2 "Badge.*variant=[\"\'](?:purple|success)" "src/components/Patient/ManagePatients.tsx"
Length of output: 217 Script: #!/bin/bash
# Let's try a simpler search first to find any Badge usage
rg -l "Badge" "src/components/Patient/ManagePatients.tsx"
# Also search for Badge imports
rg "import.*Badge" "src/components/Patient/ManagePatients.tsx"
# Let's also check if ManagePatients.tsx exists in a different location
fd -t f "ManagePatients.tsx"
Length of output: 334 Script: #!/bin/bash
# Now that we confirmed the file exists and imports Badge, let's look for its usage
rg -A 3 "<Badge" "src/components/Patient/ManagePatients.tsx"
# Also check for any variant prop usage with Badge
rg -A 3 "Badge.*variant=" "src/components/Patient/ManagePatients.tsx"
# Let's also check the entire component to understand the review status context
rg -A 5 "review.*status|status.*review" "src/components/Patient/ManagePatients.tsx" -i
Length of output: 382 Script: #!/bin/bash
# Let's get more context around the Badge usage we found
rg -B 2 -A 5 "<Badge" "src/components/Patient/ManagePatients.tsx"
# Also search for any review-related logic near the Badge
rg -B 5 -A 5 "review_time" "src/components/Patient/ManagePatients.tsx"
Length of output: 1736 🧰 Tools🪛 eslint[error] 15-16: Delete (prettier/prettier) Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/components/Patient/ManagePatients.tsx (1)
642-658: Consider using i18n for review notification textThe implementation nicely handles both missed and upcoming reviews with clear time indicators. However, the text strings should be internationalized for consistency.
Consider using i18n translations:
- ? `Review Missed (${Math.abs(dayjs().diff(dayjs(patient.review_time), "days"))} days ago)` - : `Review Due in (${Math.abs(dayjs(patient.review_time).diff(dayjs(), "days"))} days)` + ? t("review_missed_days_ago", { + days: Math.abs(dayjs().diff(dayjs(patient.review_time), "days")) + }) + : t("review_due_in_days", { + days: Math.abs(dayjs(patient.review_time).diff(dayjs(), "days")) + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(1 hunks)src/components/Patient/ManagePatients.tsx(2 hunks)
🔇 Additional comments (1)
src/components/Patient/ManagePatients.tsx (1)
Line range hint 631-641: LGTM! Good cleanup of commented code.
Removing commented-out code improves code maintainability.
package.json
Outdated
| @@ -96,7 +96,7 @@ | |||
| "tailwind-merge": "^2.5.4", | |||
| "tailwindcss-animate": "^1.0.7", | |||
| "use-keyboard-shortcut": "^1.1.6", | |||
| "xlsx": "^0.18.5" | |||
| "xlsx": "https://cdn.sheetjs.com/xlsx-0.20.3/xlsx-0.20.3.tgz" | |||
There was a problem hiding this comment.
@Sancharisingh this change seems to be unrelated to the linked PR. If you meant to solve #9183, please raise a separate PR solving that alone.
There was a problem hiding this comment.
@rithviknishad, I’ve removed the changes related to SheetJS. Please review now and let me know if further adjustments are needed.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| @@ -628,7 +628,7 @@ export const PatientManager = () => { | |||
| )} | |||
| </> | |||
| )} | |||
| {patient.review_time && | |||
| {/* {patient.review_time && | |||
There was a problem hiding this comment.
Remove the commented out/old code.
| {patient.review_time && | ||
| !patient.last_consultation?.discharge_date && | ||
| Number(patient.last_consultation?.review_interval) > 0 && ( | ||
| <Chip |
There was a problem hiding this comment.
Let's switch to using Shadcn's Badge here. You can port over the styles from Chip.
Edit: https://github.com/ohcnetwork/care_fe/pull/9138/files#diff-6d99361c2e374eb2dd472d6136d9d5929c8f32128cbb0b6ab8bdafea6beca6ae adds the badge; copy that over and add/adjust variant as necessary.
|
Also make sure to edit the title to be human friendly 👍 |
|
I’ve made the required changes. Should I go ahead and raise a new PR for them, or should I update the existing one instead? Let me know how to proceed. |
|
update existing PR |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/components/ui/badge.tsx (1)
6-26: Consider accessibility and contrast improvements.While the variants are well-defined, consider the following improvements:
- The purple variant's hover state in dark mode (
dark:hover:bg-purple-800) might have insufficient contrast with the text color (dark:text-purple-200).- Consider adding a success variant for positive states, which would be useful for completed reviews.
const badgeVariants = cva( "inline-flex items-center rounded-md border border-gray-200 px-2.5 py-0.5 text-xs font-semibold transition-colors focus:outline-none focus:ring-2 focus:ring-gray-950 focus:ring-offset-2 dark:border-gray-800 dark:focus:ring-gray-300", { variants: { variant: { default: "border-transparent bg-gray-900 text-gray-50 shadow hover:bg-gray-900/80 dark:bg-gray-50 dark:text-gray-900 dark:hover:bg-gray-50/80", secondary: "border-transparent bg-gray-100 text-gray-900 hover:bg-gray-100/80 dark:bg-gray-800 dark:text-gray-50 dark:hover:bg-gray-800/80", destructive: "border-transparent bg-red-500 text-gray-50 shadow hover:bg-red-500/80 dark:bg-red-900 dark:text-gray-50 dark:hover:bg-red-900/80", outline: "text-gray-950 dark:text-gray-50", purple: - "border-transparent bg-purple-200 text-purple-800 shadow hover:bg-purple-300 dark:bg-purple-900 dark:text-purple-200 dark:hover:bg-purple-800", + "border-transparent bg-purple-200 text-purple-800 shadow hover:bg-purple-300 dark:bg-purple-900 dark:text-purple-100 dark:hover:bg-purple-900", + success: + "border-transparent bg-green-500 text-gray-50 shadow hover:bg-green-500/80 dark:bg-green-900 dark:text-gray-50 dark:hover:bg-green-900/80", }, }, defaultVariants: { variant: "default", }, }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/ui/badge.tsx(1 hunks)
🔇 Additional comments (2)
src/components/ui/badge.tsx (2)
1-4: LGTM! Clean and well-organized imports.
The imports follow React best practices and properly separate external and internal dependencies.
1-38: Verify badge variants match all review states.
The component appears well-suited for the PR objectives, supporting both upcoming and missed review states. However, let's verify all required states are covered.
✅ Verification successful
Based on the code review, I can see that the Badge component is used with both "purple" and "destructive" variants for review states in ManagePatients.tsx. Let me verify if all the review states are properly handled.
Badge variants correctly support all review states
The implementation properly handles all review states with appropriate variants:
- "Review Missed" state uses the purple variant with "X days ago" message
- "Review Due" state uses the destructive variant with "Due in X days" message
The variants are correctly chosen based on the review time comparison, and the time differences are properly calculated using dayjs for both past and future reviews.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if ManagePatients.tsx uses appropriate badge variants for all review states
# Search for badge usage in ManagePatients
rg -A 3 "Badge.*variant=" "src/components/Patient/ManagePatients.tsx"
# Search for review-related text to ensure all states are handled
rg "review" "src/components/Patient/ManagePatients.tsx"
Length of output: 638
Script:
#!/bin/bash
# Let's check the actual implementation of badge usage in ManagePatients.tsx to see the variants being used
ast-grep --pattern 'Badge variant=$_'
# Also check for any badge-related JSX elements
rg -l "<Badge" src/components/Patient/ManagePatients.tsx | xargs cat
Length of output: 45147
Script:
#!/bin/bash
# Let's check the review time logic implementation
ast-grep --pattern 'dayjs().isAfter(patient.review_time) ? $_ : $_'
Length of output: 1086
src/components/ui/badge.tsx
Outdated
| export interface BadgeProps | ||
| extends React.HTMLAttributes<HTMLDivElement>, | ||
| VariantProps<typeof badgeVariants> {} | ||
|
|
||
| function Badge({ className, variant, ...props }: BadgeProps) { | ||
| return ( | ||
| <div className={cn(badgeVariants({ variant }), className)} {...props} /> | ||
| ); | ||
| } | ||
|
|
||
| export { Badge, badgeVariants }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and documentation.
The component implementation is clean, but could benefit from improved accessibility and documentation:
+/**
+ * Badge component for displaying status indicators.
+ * @param variant - The visual style variant of the badge
+ * @param className - Additional CSS classes to apply
+ */
export interface BadgeProps
extends React.HTMLAttributes<HTMLDivElement>,
VariantProps<typeof badgeVariants> {}
function Badge({ className, variant, ...props }: BadgeProps) {
return (
<div
+ role="status"
+ aria-label={props['aria-label'] || props.children?.toString()}
className={cn(badgeVariants({ variant }), className)}
{...props}
/>
);
}This change:
- Adds TypeScript documentation for better developer experience
- Adds proper ARIA role for accessibility
- Adds aria-label with fallback to child content
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface BadgeProps | |
| extends React.HTMLAttributes<HTMLDivElement>, | |
| VariantProps<typeof badgeVariants> {} | |
| function Badge({ className, variant, ...props }: BadgeProps) { | |
| return ( | |
| <div className={cn(badgeVariants({ variant }), className)} {...props} /> | |
| ); | |
| } | |
| export { Badge, badgeVariants }; | |
| /** | |
| * Badge component for displaying status indicators. | |
| * @param variant - The visual style variant of the badge | |
| * @param className - Additional CSS classes to apply | |
| */ | |
| export interface BadgeProps | |
| extends React.HTMLAttributes<HTMLDivElement>, | |
| VariantProps<typeof badgeVariants> {} | |
| function Badge({ className, variant, ...props }: BadgeProps) { | |
| return ( | |
| <div | |
| role="status" | |
| aria-label={props['aria-label'] || props.children?.toString()} | |
| className={cn(badgeVariants({ variant }), className)} | |
| {...props} | |
| /> | |
| ); | |
| } | |
| export { Badge, badgeVariants }; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/components/ui/badge.tsx (1)
6-10: Enhance component documentation for review status use case.While the basic documentation is good, consider adding more details about the component's role in displaying review status indicators.
/** * Badge component for displaying status indicators. * @param variant - The visual style variant of the badge * @param className - Additional CSS classes to apply + * @example + * // Usage for review status + * <Badge variant="destructive">Review Missed</Badge> + * <Badge variant="success">Review Due in 2 days</Badge> */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/ui/badge.tsx(1 hunks)
🔇 Additional comments (3)
src/components/ui/badge.tsx (3)
12-34: Well-structured variants with appropriate styling.
The badge variants are well-designed with:
- Appropriate color choices for review status (success/destructive)
- Consistent dark mode support
- Proper accessibility features (focus states)
40-49: Implementation matches previous accessibility recommendations.
The current implementation already includes the accessibility improvements suggested in the previous review (role="status" and aria-label).
1-52: Verify integration with review status display requirements.
Let's confirm this component is properly integrated with the review status display requirements.
✅ Verification successful
Badge component is correctly integrated with review status requirements
The verification confirms that the Badge component is properly integrated for displaying review status:
- The component is used to show review status with appropriate variants:
- Uses "purple" variant for "Review Missed" cases
- Uses "destructive" variant for upcoming reviews
- Shows detailed time information:
- For missed reviews: "Review Missed X days ago"
- For upcoming reviews: "Review Due in X days"
- Only displays for active patients with review intervals > 0 and no discharge date
The implementation aligns with the review status display requirements and uses the Badge component's variants appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Badge component usage in ManagePatients.tsx
# Expect to find implementations for both "Review Missed" and "Time left for review" cases
rg -A 5 "Badge.*variant=(\"|\')?(destructive|success)" "src/components/Patient/ManagePatients.tsx"
Length of output: 228
Script:
#!/bin/bash
# Search for Badge component usage in ManagePatients.tsx with fixed regex
rg -A 5 "Badge.*variant" "src/components/Patient/ManagePatients.tsx"
# Also search for Badge imports to verify integration
rg "import.*Badge" "src/components/Patient/ManagePatients.tsx"
# Search for Badge usage across the codebase to understand implementation patterns
rg -A 5 "Badge.*variant"
Length of output: 661
Script:
#!/bin/bash
# Let's search for actual usage of Badge component in ManagePatients.tsx
rg -l "Badge" "src/components/Patient/ManagePatients.tsx" | xargs cat
# Also search for any review status related code in ManagePatients.tsx
rg -A 5 "review|status" "src/components/Patient/ManagePatients.tsx"
# Search for FilterBadge usage since it's also imported
rg -A 5 "FilterBadge" "src/components/Patient/ManagePatients.tsx"
Length of output: 47572
|
I made the required changes please review my code. |
|
👋 Hi, @Sancharisingh, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
@Jacobjeevan can you look into the PR |
| </div> | ||
|
|
||
| <SearchByMultipleFields |
There was a problem hiding this comment.
Merge error here; SearchByMultipleFields should be included instead of the individual search fields.
| const onlyAccessibleFacility = | ||
| permittedFacilities?.count === 1 ? permittedFacilities.results[0] : null; | ||
|
|
||
| const searchOptions = [ |
There was a problem hiding this comment.
Merge error; Part of SearchByMultipleFields, and as such should be included.
Jacobjeevan
removed
needs testing
needs review
Deploy-Failed
|
Closing as stale, feel free to reopen once changes made. |
|
Hi @rithviknishad, |
Proposed Changes
Time left for review if the review is upcoming.
Missed since [X] days if the review time has passed
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit